-
Notifications
You must be signed in to change notification settings - Fork 313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add percentage diff column to compare subcommand #1333
Conversation
FYI @pquentin, I liked your suggestion on statistically significant highlights, but perhaps better tackled in a seperate PR :-) I just want a quick way to see diff % at a glance for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks, that will help us a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change. I left a proposal for a different implementation of the _diff
method. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
] | ||
else: | ||
return [] | ||
|
||
def _diff(self, baseline, contender, treat_increase_as_improvement, formatter=lambda x: x): | ||
def _diff(self, baseline, contender, treat_increase_as_improvement, formatter=lambda x: x, as_percentage=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, on my first review, I hesitated to mention that adding a boolean parameter to a function is usually considered to be a code smell. But now that Daniel suggested a refactoring, I'm less shy. :) https://adamj.eu/tech/2021/07/10/python-type-hints-how-to-avoid-the-boolean-trap/ explains why that's a problem and what the alternatives are in modern Python.
(It's fine in this case as it's a small private function only called twice: I'm not asking to change your code. But maybe you'll find those references useful.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the references! They're great. It's funny because my first (uncommitted) approach was actually to implement this as a seperate private function called diff_percentage
, but I was worried that there was too much code duplication.
Adds a new column showing the percentage difference between a
baseline
andcontender
.Closes #1328
Looks like:
Example compare